Assuming a memory profiling extension, registering observers during MINIT adds unnecessary overhead when not profiling.

@arnaud-lb The overhead would be only once per ginit, with a relatively low overhead. I think that's pretty reasonable.

The alternative would be that memory profiling extensions have to register in each rinit, and PHP would have to free these in rshutdowns (or at least check for it, even if nothing happens). Not sure whether doing that would really have less overhead.

@arnaud-lb The overhead would be only once per ginit, with a relatively low overhead. I think that's pretty reasonable.

The alternative would be that memory profiling extensions have to register in each rinit, and PHP would have to free these in rshutdowns (or at least check for it, even if nothing happens). Not sure whether doing that would really have less overhead.

I was referring to overhead in the allocation functions, since they take the "custom heap" path when some observers are enabled. But I was missing that extensions would disable the observers when they are not needed. As long as they do, this will have no overhead, so that's fine.

Is there a particular reason for requiring that observers are registered during startup/MINIT? Assuming a memory profiling extension, registering observers during MINIT adds unnecessary overhead when not profiling.

Probably because zend_mm_observers is a true global, and thus we'd need some synchronization locking for adding new observers. E.g.

1. Allocate new zend_mm_heap_observer
2. Fetch current zend_mm_observers global
3. Assign current zend_mm_observers to zend_mm_heap_observer.next
4. Assign new zend_mm_heap_observer to zend_mm_observers global

We can have a race condition between steps 2 and 4, losing the value from the thread that finished first. Furthermore, we'd need to move this to a shared global for NTS. If we could register observers dynamically we would likely also need to offer API to unregister them.

As you mentioned, an extension that registers an observer could disable it by default unless profiling was requested.

Edit: Oh, they are appended. But the issue remains.

Is there a particular reason for requiring that observers are registered during startup/MINIT? Assuming a memory profiling extension, registering observers during MINIT adds unnecessary overhead when not profiling.

Probably because zend_mm_observers is a true global, and thus we'd need some synchronization locking for adding new observers. E.g.

I got that we can not mutate zend_mm_observers due to concurrency, but does zend_mm_observers need to be a true global? This avoids allocations because observers are registered once, but this adds complexity:

• zend_mm_observers need to be copied to thread-locals because they are mutated by the enable/disable APIs, and because we need the enabled/disabled state to be thread-local.
• The lifetime of the copy spans multiple requests, so the enabled/disabled state persists across requests. This requires the extension to take care of checking or updating the state of its observers at the beginning of each request.

Couldn't we obtain the same functionality if zend_mm_observers was thread-local, and observers was registered just for one request? This requires one malloc/free per observer (are extension expected to register more than one?), but this may not be significant compared to the overhead of calling the observer (even with a no-op observer).

@arnaud-lb Of course, in my comment I missed that observers can be enabled/disabled per request and so can't be shared across processes/threads. Your suggestion to enable per request sounds simpler. The overhead of the allocation per request is negligible, and only relevant when memory profiling. Memory profiling leaking across requests doesn't seem particularly useful.

Hey @arnaud-lb and @iluuu1994,
thanks for your feedback 👍 (and sorry for not being very responsive, I was sick yesterday)
You are saying that we could make this easier by calling zend_mm_observers_register() in RINIT (and store observers directly on the AG(mm_heap)->observers list) and a zend_mm_observers_unregister() in RSHUTDOWN? (we may also omit the unregister and make this internally in the zend_mm_shutdown() when full == false)
This would solve some of the problems you mentioned, especially the syncing from global to thread local. Having the observers stay active across requests was intentional as we'd save an allocation, the corresponding free and traversing the linked list per request. I do understand that you say this is negligible (I personally was not sure about this), so I'll change the implementation to this:

• zend_mm_observers_register() in RINIT (passing malloc, free, and realloc function pointers)
  • register directly to the AG(mm_heap)->observers
  • set AG(mm_heap)->use_custom_heap flag
• unregister in zend_mm_shutdown() when full == false
  • free observers list
  • unset AG(mm_heap)->use_custom_heap flag

I'll play with this and check the usage, but I think it makes sense this way especially when the malloc() and the free() are negligible.
About the unregister: I'll play with this and see. When we say an extension needs to call the unregister, we should make sure to unregister internally before extension are dlclose()ed, as there are some free's happening after dlclose() and an extension could just not unregister and will crash. Unregistering in zend_mm_shutdown() would add a NULL check on the AG(mm_heap)->observers for everyone, probably also negligible?

What do you think?

One thing in favour of having a unregister function is that this would allow an extension to expose observing memory allocations to userspace (add a memprof_enable() and memprof_disable() function to PHP) and enable / disable at will. We may not use it this way, but it would be certainly possible!
It currently is possible using the zend_mm_set_custom_handlers() API.

Hey @arnaud-lb and @iluuu1994, thanks for your feedback +1 (and sorry for not being very responsive, I was sick yesterday)

No worries, and I hope you are feeling better now!

You are saying that we could make this easier by calling zend_mm_observers_register() in RINIT (and store observers directly on the AG(mm_heap)->observers list) and a zend_mm_observers_unregister() in RSHUTDOWN? (we may also omit the unregister and make this internally in the zend_mm_shutdown() when full == false) This would solve some of the problems you mentioned, especially the syncing from global to thread local. Having the observers stay active across requests was intentional as we'd save an allocation, the corresponding free and traversing the linked list per request. I do understand that you say this is negligible (I personally was not sure about this), so I'll change the implementation to this:

* `zend_mm_observers_register()` in RINIT (passing malloc, free, and realloc function pointers)
  
  * register directly to the `AG(mm_heap)->observers`
  * set `AG(mm_heap)->use_custom_heap` flag

* unregister in `zend_mm_shutdown()` when `full == false`
  
  * free observers list
  * unset `AG(mm_heap)->use_custom_heap` flag

I'll play with this and check the usage, but I think it makes sense this way especially when the malloc() and the free() are negligible. About the unregister: I'll play with this and see. When we say an extension needs to call the unregister, we should make sure to unregister internally before extension are dlclose()ed, as there are some free's happening after dlclose() and an extension could just not unregister and will crash. Unregistering in zend_mm_shutdown() would add a NULL check on the AG(mm_heap)->observers for everyone, probably also negligible?

What do you think?

Thank you for these changes. This is simpler and it's probably easier to use the API in the right way as well.

Agreed that we should unregister observers in mm_shutdown (including when full=false), as this would also prevent accidentally leaking observers to next requests.

I will try the API on memprof later this week (probably on Friday) to check that it's suitable for more than one extension.

I do think we should have some simple tests in ext/zend_test.